001 staff booking flow#39
Conversation
|
Thank you for following naming conventions! 😻 |
Deploying ev-rental with
|
| Latest commit: |
1846931
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9f7a78a0.ev-rental.pages.dev |
| Branch Preview URL: | https://001-staff-booking-flow.ev-rental.pages.dev |
nnh53
force-pushed
the
001-staff-booking-flow
branch
from
9d95f45 to
809e94f
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a comprehensive staff booking fulfillment workflow allowing staff to process bookings through a mandatory sequential API flow: check-in → rental creation → contract generation → inspection → dual signatures → vehicle handover. The implementation adds a dedicated fulfillment route with a new orchestration layer in core-logic/rental-fulfillment and UI components under features/staff/booking-fulfillment.
Key Changes:
- New fulfillment orchestration service with signal-based state management enforcing step dependencies
- Dedicated fulfillment page with step checklist UI, signature capture, and inspection forms
- Extended staff dashboard with "Tiếp tục xử lý" CTA linking to fulfillment route
- Analytics service tracking step completion/failures with timeline audit trail
Reviewed Changes
Copilot reviewed 37 out of 38 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
src/app/features/staff/booking-fulfillment/pages/fulfillment-page/fulfillment-page.ts |
Main fulfillment page component orchestrating 6-step sequential workflow |
src/app/core-logic/rental-fulfillment/fulfillment.service.ts |
Orchestrator coordinating API calls with error handling and step state management |
src/app/core-logic/rental-fulfillment/fulfillment.state.ts |
Signal-based state store managing step progression and artifacts |
src/app/features/staff/staff-dashboard/staff-dashboard.ts |
Enhanced dashboard with fulfillment CTA and improved change detection |
src/app/features/staff/booking-fulfillment/components/inspection-form/inspection-form.ts |
Reactive form for battery capacity and inspection evidence capture |
src/app/features/staff/booking-fulfillment/components/signature-step/signature-step.ts |
Dual-role signature capture component for renter/staff |
src/app/core-logic/bookings/bookings.service.ts |
Extended with fulfillment summary loader and timeline builder |
src/app/layout/layout.html |
Fixed @for track expressions using unique identifiers |
Comments suppressed due to low confidence (1)
src/app/features/staff/booking-fulfillment/pages/fulfillment-page/fulfillment-page.ts:1
- This method appears in
staff-dashboard.tsbut the diff shows it at line 609 infulfillment-page.ts. Verify the correct file location. If duplicated across files, extract to a shared utility since both components need stable card keys for@fortracking.
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| canOpenFulfillment(record: StaffBookingRecord): boolean { | ||
| return true; | ||
| // return ( | ||
| // record.verificationStatus === BookingVerificationStatusEnum.Approved || | ||
| // record.status === BookingStatusEnum.Verified || | ||
| // record.status === BookingStatusEnum.RentalCreated | ||
| // ); | ||
| } |
There was a problem hiding this comment.
The canOpenFulfillment method bypasses business logic by always returning true, making the unused parameter directive necessary. Either implement the commented validation logic or remove the method entirely if fulfillment access should be unrestricted. The current state creates dead code and misleads readers about intended constraints.
| effect(() => { | ||
| this.bookingsService.staffBookings(); | ||
| this.bookingsService.staffBookingsLoading(); | ||
| this.bookingsService.staffBookingsError(); | ||
| queueMicrotask(() => this.cdr.detectChanges()); | ||
| }); |
There was a problem hiding this comment.
Manual detectChanges() in an effect contradicts Angular 20's zoneless change detection philosophy. Signal reactivity should automatically trigger template updates without explicit ChangeDetectorRef calls. If templates aren't updating, investigate why signals aren't properly connected rather than working around it with manual detection. This pattern undermines the benefits of the zoneless architecture documented in guideline 1000000.
| effect(() => { | ||
| this.orchestrator.snapshot(); | ||
| this.initializationError(); | ||
| this.routeEntered(); | ||
| queueMicrotask(() => this.cdr.detectChanges()); | ||
| }); |
There was a problem hiding this comment.
Similar to staff-dashboard, this effect manually triggers change detection via ChangeDetectorRef which defeats zoneless signal reactivity. The signals accessed here (snapshot, initializationError, routeEntered) should already notify Angular of changes. Remove this workaround and ensure computed/signal dependencies are properly declared.
| private _resolveTokenUserId(): string | undefined { | ||
| const accessToken = this._tokenService.accessToken.token; | ||
| const token = this._normalizeString(accessToken); | ||
| if (!token) { | ||
| return undefined; | ||
| } | ||
|
|
||
| try { | ||
| const payload = this._tokenService.decodeToken(token); | ||
| const staffId = this._normalizeString(payload.StaffId); | ||
| if (staffId) { | ||
| return staffId; | ||
| } | ||
|
|
||
| const subject = this._normalizeString(payload.sub); | ||
| if (subject) { | ||
| return subject; | ||
| } | ||
|
|
||
| const rawPayload = payload as Record<string, unknown>; | ||
| const nameIdentifierKey = | ||
| 'http://schemas.xmlsoap.org/ws/2005/05/identity/claims/nameidentifier'; | ||
| const nameIdentifierClaim = this._normalizeString( | ||
| typeof rawPayload[nameIdentifierKey] === 'string' | ||
| ? (rawPayload[nameIdentifierKey] as string) | ||
| : undefined, | ||
| ); | ||
|
|
||
| return nameIdentifierClaim; | ||
| } catch { | ||
| return undefined; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Complex token claim resolution logic with hardcoded XML namespace string should be extracted to TokenService rather than duplicated here. This creates tight coupling to JWT internals and duplicates the fallback chain (StaffId → sub → nameidentifier). Refactor to tokenService.resolveUserId() to centralize token parsing logic.
| private _resolveTokenUserId(): string | undefined { | |
| const accessToken = this._tokenService.accessToken.token; | |
| const token = this._normalizeString(accessToken); | |
| if (!token) { | |
| return undefined; | |
| } | |
| try { | |
| const payload = this._tokenService.decodeToken(token); | |
| const staffId = this._normalizeString(payload.StaffId); | |
| if (staffId) { | |
| return staffId; | |
| } | |
| const subject = this._normalizeString(payload.sub); | |
| if (subject) { | |
| return subject; | |
| } | |
| const rawPayload = payload as Record<string, unknown>; | |
| const nameIdentifierKey = | |
| 'http://schemas.xmlsoap.org/ws/2005/05/identity/claims/nameidentifier'; | |
| const nameIdentifierClaim = this._normalizeString( | |
| typeof rawPayload[nameIdentifierKey] === 'string' | |
| ? (rawPayload[nameIdentifierKey] as string) | |
| : undefined, | |
| ); | |
| return nameIdentifierClaim; | |
| } catch { | |
| return undefined; | |
| } | |
| } | |
| // User ID resolution is now delegated to TokenService.resolveUserId() |
| <button | ||
| type="submit" | ||
| class="vehicle-receive__submit" | ||
| [disabled]="step.disabled || vehicleReceiveForm.invalid" |
There was a problem hiding this comment.
The disabled binding combines orchestrator state (step.disabled) with form validity, but step.disabled already accounts for busy state and prerequisites. This creates redundant validation since the form is disabled when step.disabled is true (via effect at lines 288-293). Simplify to [disabled]=\"step.disabled\" to avoid confusion.
| this._steps.update((steps) => { | ||
| const nextSteps: FulfillmentStepState[] = []; | ||
| for (const step of steps) { | ||
| if (step.step !== stepId) { | ||
| nextSteps.push(step); | ||
| continue; | ||
| } | ||
|
|
||
| nextSteps.push({ | ||
| ...step, | ||
| ...changes, | ||
| requires: changes.requires ?? step.requires, | ||
| artifact: changes.artifact ?? step.artifact, | ||
| error: changes.error, | ||
| }); | ||
| } | ||
| return nextSteps; | ||
| }); |
There was a problem hiding this comment.
[nitpick] The loop always rebuilds the entire steps array even when updating a single step. For the 7-step fulfillment workflow this is acceptable, but consider using .map() for clearer intent: steps.map(step => step.step === stepId ? { ...step, ...changes, ... } : step). This makes the transformation more declarative and easier to understand.
| this._steps.update((steps) => { | |
| const nextSteps: FulfillmentStepState[] = []; | |
| for (const step of steps) { | |
| if (step.step !== stepId) { | |
| nextSteps.push(step); | |
| continue; | |
| } | |
| nextSteps.push({ | |
| ...step, | |
| ...changes, | |
| requires: changes.requires ?? step.requires, | |
| artifact: changes.artifact ?? step.artifact, | |
| error: changes.error, | |
| }); | |
| } | |
| return nextSteps; | |
| }); | |
| this._steps.update((steps) => | |
| steps.map((step) => | |
| step.step === stepId | |
| ? { | |
| ...step, | |
| ...changes, | |
| requires: changes.requires ?? step.requires, | |
| artifact: changes.artifact ?? step.artifact, | |
| error: changes.error, | |
| } | |
| : step, | |
| ) | |
| ); |
| <button | ||
| type="button" | ||
| class="details-panel__action" | ||
| (click)="openFulfillment(detail.record)" | ||
| [disabled]="!canOpenFulfillment(detail.record)" | ||
| > | ||
| Tiếp tục xử lý | ||
| </button> |
There was a problem hiding this comment.
The button lacks aria-label or aria-describedby to explain what 'Tiếp tục xử lý' means in context. Screen reader users won't know this continues processing for the specific booking. Add [attr.aria-label]=\"'Tiếp tục xử lý booking ' + detail.record.bookingId\" to provide context.
nnh53
force-pushed
the
001-staff-booking-flow
branch
from
809e94f to
1846931
Compare
💡 Codex ReviewEV-Rental-FE/src/app/core-logic/bookings/bookings.service.ts Lines 264 to 333 in 9d95f45  Persist post-contract steps in fulfillment timeline
The fulfillment store restores step state after a refresh by scanning timeline events (see ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
No description provided.